Logic to handle optional_extra in Vision Excel converter input #344

Jerry-Jinfeng-Guo · 2025-11-26T11:06:12Z

Closes #338

In this PR:

Logic to handle optional_extra
Documentation
Test

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Copilot

Pull request overview

This PR implements logic to handle optional extra columns in the tabular converter, addressing issue #338. The feature allows specifying columns that should be included in extra_info if present but won't cause conversion failure if missing.

Key Changes:

Added allow_missing parameter throughout the column definition parsing chain to support optional columns
Implemented optional_extra wrapper in column definitions to mark columns as optional
Enhanced error handling to gracefully skip missing optional columns while preserving required column behavior

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/power_grid_model_io/converters/tabular_converter.py	Core implementation of optional_extra logic with allow_missing parameter propagation and empty DataFrame handling for missing columns
tests/unit/converters/test_tabular_converter.py	Comprehensive test coverage for optional_extra feature including edge cases and integration tests
docs/converters/vision_converter.md	Documentation explaining optional_extra syntax, behavior, and use cases for Vision Excel exports

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/unit/converters/test_tabular_converter.py

src/power_grid_model_io/converters/tabular_converter.py

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 13 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/power_grid_model_io/converters/tabular_converter.py

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

mgovers · 2025-12-01T07:57:37Z

docs/converters/vision_converter.md

+      extra:
+        - ID            # Required - fails if missing
+        - Name          # Required - fails if missing
+        - optional_extra:


what happens if an element is specified in both the required and optional?

extra: - ID # Required - fails if missing - optional_extra: - ID # Optional - skipped if missing

I believe that the default should be that required precedes optional, so maybe we need to add an explicit test case for this.

src/power_grid_model_io/converters/tabular_converter.py

mgovers · 2025-12-01T08:46:08Z

src/power_grid_model_io/converters/tabular_converter.py

+        allow_missing: bool = False,
    ) -> pd.DataFrame:
        """Interpret the column definition and extract/convert/create the data as a pandas DataFrame.

        Args:
          data: TabularData:
          table: str:
          col_def: Any:
          extra_info: Optional[ExtraInfo]:
+          allow_missing: bool: If True, missing columns will return empty DataFrame instead of raising KeyError


I wonder whether this is the right solution. If I understand correctly, this parameter is needed due to the recursion, right? Would there be a world in which either no recursion is needed, or in which we can do without this allow_missing? It feels bugprone

#344 (comment)

mgovers · 2025-12-01T09:08:29Z

src/power_grid_model_io/converters/tabular_converter.py

+        allow_missing: bool = False,
    ) -> pd.DataFrame:
        """Interpret the column definition and extract/convert/create the data as a pandas DataFrame.

        Args:
          data: TabularData:
          table: str:
          col_def: Any:
          extra_info: Optional[ExtraInfo]:
+          allow_missing: bool: If True, missing columns will return empty DataFrame instead of raising KeyError


please add allow_missing to the tests (either by testing that it works as intended + that all sub-calls to the mocks are made correctly, and/or by raising an error if it is set but recursion level is not 0)

mgovers · 2025-12-01T09:09:50Z

src/power_grid_model_io/converters/tabular_converter.py

        col_def: Any,
        table_mask: Optional[np.ndarray],
        extra_info: Optional[ExtraInfo],
+        allow_missing: bool = False,


please make a keyword arg

Suggested change

allow_missing: bool = False,

*,

allow_missing: bool = False,

optionally, you can even demand that the other fields are keyword args as well (that's OK because it's a protected member function so we can change the interface without any issues)

If it's a private function and we can change it at will. Does it really matter if we have allow_missing as kwarg? It is a matter of preference? Is it best practice? Or is there other reason? I don't have a strong opinion, but I wish to understand better.

the reason is that it's very bugprone unless we thoroughly test this

mgovers · 2025-12-01T09:21:52Z

tests/unit/converters/test_tabular_converter.py

please add a test that check the behavior when an attribute is added to both extra and optional_extra, e.g.

extra: - ID - optional_extra: - ID

figueroa1395 · 2025-12-01T09:56:35Z

src/power_grid_model_io/converters/tabular_converter.py

        col_def: Any,
        table_mask: Optional[np.ndarray],
        extra_info: Optional[ExtraInfo],
+        allow_missing: bool = False,


If it's a private function and we can change it at will. Does it really matter if we have allow_missing as kwarg? It is a matter of preference? Is it best practice? Or is there other reason? I don't have a strong opinion, but I wish to understand better.

figueroa1395 · 2025-12-01T10:08:36Z

src/power_grid_model_io/converters/tabular_converter.py

+                return self._parse_col_def_composite(
+                    data=data, table=table, col_def=optional_cols, table_mask=table_mask, allow_missing=True
+                )


This is the only place where I see allow_missing=True. Why is that? Is it because it's only relevant when the underlying structure is a dict, hence then is only when the "new" extra optional parameters matter? Why isn't it relevant bellow when we have a list instead?

I don't think I've ever worked in this repo, so genuinely asking. I'd appreciate tech review over this :)

For now only optional_extra are fields we attribute the 'optional' to its existence.

Comments from Martijn Co-authored-by: Martijn Govers <martijn.govers@alliander.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

sonarqubecloud · 2025-12-01T20:30:32Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
95.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

logic to handle optional_extra

60011ca

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Jerry-Jinfeng-Guo requested a review from Copilot November 26, 2025 11:06

Jerry-Jinfeng-Guo self-assigned this Nov 26, 2025

Jerry-Jinfeng-Guo added the feature New feature or request label Nov 26, 2025

Jerry-Jinfeng-Guo changed the title ~~logic to handle optional_extra~~ Logic to handle optional_extra in Vision Excel converter input Nov 26, 2025

Copilot AI reviewed Nov 26, 2025

View reviewed changes

tests/unit/converters/test_tabular_converter.py Show resolved Hide resolved

src/power_grid_model_io/converters/tabular_converter.py Outdated Show resolved Hide resolved

Jerry-Jinfeng-Guo and others added 3 commits November 26, 2025 12:08

ignored file

7f00482

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Update src/power_grid_model_io/converters/tabular_converter.py

5d78037

Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

added tests for optional_extra fields

e375135

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Jerry-Jinfeng-Guo requested a review from Copilot November 27, 2025 13:30

Copilot AI reviewed Nov 27, 2025

View reviewed changes

src/power_grid_model_io/converters/tabular_converter.py Outdated Show resolved Hide resolved

src/power_grid_model_io/converters/tabular_converter.py Outdated Show resolved Hide resolved

Jerry-Jinfeng-Guo and others added 4 commits November 27, 2025 14:35

append unit test for coverage

d357c8a

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Update src/power_grid_model_io/converters/tabular_converter.py

ef1ce32

Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

Update src/power_grid_model_io/converters/tabular_converter.py

826b266

Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

remove unused variable

a9bf00e

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

mgovers reviewed Dec 1, 2025

View reviewed changes

figueroa1395 reviewed Dec 1, 2025

View reviewed changes

Jerry-Jinfeng-Guo and others added 3 commits December 1, 2025 21:09

Update src/power_grid_model_io/converters/tabular_converter.py

c9b3816

Comments from Martijn Co-authored-by: Martijn Govers <martijn.govers@alliander.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

Update src/power_grid_model_io/converters/tabular_converter.py

1e4b625

Comments from Martijn Co-authored-by: Martijn Govers <martijn.govers@alliander.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

made kwarg; add specfic tests

2f84ac9

Signed-off-by: Jerry Guo <Jerry.Jinfeng.Guo@alliander.com>

Logic to handle optional_extra in Vision Excel converter input #344

Are you sure you want to change the base?

Logic to handle optional_extra in Vision Excel converter input #344

Uh oh!

Conversation

Jerry-Jinfeng-Guo commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 1, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jerry-Jinfeng-Guo commented Nov 26, 2025 •

edited

Loading